Sources/AblyChat/DefaultTyping.swift (2)

9-9: Fix typo in property name.

The property name contains a typo: "Presense" should be "Presence".

-    private let maxPresenseGetRetryDuration: TimeInterval // Max duration as specified in CHA-T6c1
+    private let maxPresenceGetRetryDuration: TimeInterval // Max duration as specified in CHA-T6c1

---

242-252: Prevent memory leaks in test subscriptions.

The test subscription arrays can grow indefinitely as new subscriptions are added. Consider adding cleanup logic to remove terminated subscriptions.

Add a method to clean up terminated subscriptions:

private func cleanupTerminatedSubscriptions() {
    testStartTypingEventSubscriptions.removeAll(where: { $0.isTerminated })
    testStopTypingEventSubscriptions.removeAll(where: { $0.isTerminated })
    testPresenceGetTypingEventSubscriptions.removeAll(where: { $0.isTerminated })
    testPresenceGetRetryTypingEventSubscriptions.removeAll(where: { $0.isTerminated })
}

Call this method before adding new subscriptions in each testsOnly_subscribeTo* method.

Tests/AblyChatTests/DefaultRoomTypingTests.swift (3)

298-298: Implement missing test for CHA-T6c2 specification.

The test ifMultiplePresenceEventsReceivedThenOnlyTheLatestEventIsEmitted is currently unimplemented.

Would you like me to help implement this test case? I can provide a complete implementation that verifies only the latest event is emitted when multiple presence events are received.

---

268-268: Document the rationale for the arbitrary timeout value.

The comment mentions the timeout value is arbitrary and references an issue. Consider adding more context about why this specific value was chosen.

-        let maxPresenseGetRetryDuration = 3.0 // arbitrary, TODO: improve https://github.com/ably/ably-chat-swift/issues/216
+        let maxPresenseGetRetryDuration = 3.0 // Chosen to balance between quick test execution and reliable presence get retries
+        // TODO: Make this configurable or determine dynamically - https://github.com/ably/ably-chat-swift/issues/216

---

133-134: Extract magic numbers into named constants.

The timeout value of 0.5 seconds is hardcoded without explanation. Consider extracting this and other similar values into named constants with clear documentation.

+        // Default timeout is 5 seconds, using 0.5 for faster test execution
+        private static let testTimeout: TimeInterval = 0.5
+
-        let timeout = 0.5 // default is 5 (seconds)
+        let timeout = Self.testTimeout

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Sources/AblyChat/DefaultTyping.swift (1)

Line range hint 43-82: Ensure Retry Logic Correctly Respects Maximum Retry Duration

In the retry loop for presence.get(), verify that the totalElapsedTime calculation correctly accounts for the delays, including jitter, and that the loop exits after reaching maxPresenceGetRetryDuration.

Review and adjust the calculation of totalElapsedTime to ensure it accurately reflects the accumulated wait times. This ensures the retry mechanism adheres to the specified maximum duration.

Tests/AblyChatTests/DefaultRoomTypingTests.swift (6)

25-52: Ensure Proper Synchronization When Testing Attachment State

In retrieveCurrentlyTypingClientIDsWhileAttaching(), the test sets up an attaching state and waits for the room to become attaching before calling defaultTyping.get(). To prevent flaky tests, ensure that the test reliably waits for the expected state.

Consider adding explicit synchronization or wait mechanisms to ensure the room status is in the attaching state before proceeding. This can improve the reliability of the test.

---

122-149: Potential Race Condition in Timing-Sensitive Test

In usersMayConfigureTimeoutForTyping(), the test checks if the typing stopped event is emitted after a custom timeout interval. Timing-based tests can lead to flaky results due to execution environment variability.

Consider using mock timers or expectations to control the timing more deterministically. This approach can enhance the reliability of the test across different environments.

---

194-217: Adjust Tolerance for Timing Comparison

In ifTypingIsAlreadyInProgressThenTimeoutIsExtended(), the test compares intervals using a tolerance of 0.1. Due to potential execution speed variations, consider increasing the tolerance to prevent intermittent test failures.

Adjust the tolerance value to a higher number, such as 0.2, or utilize mock timers for better reliability in timing comparisons.

---

241-259: Ensure Proper Cleanup of Subscriptions in Tests

In whenPresenceEventReceivedClientWillPerformPresenceGet(), verify that any subscriptions or event listeners are properly cleaned up after the test to prevent side effects on other tests.

Add code to unsubscribe or cancel any ongoing subscriptions at the end of the test. This practice ensures test isolation and reliability.

---

263-293: Assist with Implementing the Presence Get Retry Test

The test ifPresenceGetFailsItShallBeRetriedUsingBackoffWithJitter() includes a TODO comment referencing issue #216.

Would you like assistance in implementing this test case? I can help draft the test code or provide guidance on handling the retry logic with backoff and jitter.

---

297-300: Complete the Test Implementation for Multiple Presence Events

The test ifMultiplePresenceEventsReceivedThenOnlyTheLatestEventIsEmitted() is marked as TODO.

Do you need help in implementing this test? I can assist in writing the test code to verify that only the latest typing event is emitted when multiple presence events are received.

Sources/AblyChat/DefaultTyping.swift (1)

236-282: Organize Test-Only Code for Better Maintainability

The test-only code under #if DEBUG includes several subscription methods and structures. For improved maintainability, consider organizing these methods together and providing clear documentation of their purpose.

Grouping the test-only code into an extension or a separate file (if appropriate) can enhance code readability and maintenance.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Sources/AblyChat/DefaultTyping.swift (2)

237-282: Enhance documentation for test-only APIs.

The test infrastructure is well-organized, but the documentation could be more descriptive about the purpose and usage of each subscription type.

Apply this diff to enhance the documentation:

 #if DEBUG
-    /// The `DefaultTyping` emits a `TestTypingEvent` each time ``start`` or ``stop`` is called.
+    /// Represents a typing-related test event with a timestamp.
+    /// Used to track when various typing-related operations occur during testing.
     internal struct TestTypingEvent: Equatable {
         internal let timestamp = Date()
     }

-    /// Subscription of typing start events for testing purposes.
+    /// Collection of subscriptions that receive events when typing starts.
+    /// These events are emitted after successfully entering presence.
     private var testStartTypingEventSubscriptions: [Subscription<TestTypingEvent>] = []

-    /// Subscription of typing stop events for testing purposes.
+    /// Collection of subscriptions that receive events when typing stops.
+    /// These events are emitted after successfully leaving presence.
     private var testStopTypingEventSubscriptions: [Subscription<TestTypingEvent>] = []

-    /// Subscription of presence get events for testing purposes.
+    /// Collection of subscriptions that receive events when presence.get() succeeds.
+    /// These events are emitted after successfully retrieving the presence set.
     private var testPresenceGetTypingEventSubscriptions: [Subscription<TestTypingEvent>] = []

-    /// Subscription of retry presence get events for testing purposes.
+    /// Collection of subscriptions that receive events when presence.get() fails and is retried.
+    /// These events are emitted before each retry attempt.
     private var testPresenceGetRetryTypingEventSubscriptions: [Subscription<TestTypingEvent>] = []

---

285-287: Document the rationale for @unchecked Sendable conformance.

Add documentation explaining why the conformance is needed and why it's safe to use @unchecked.

Apply this diff:

 #if DEBUG
+/// Conformance to Sendable is required for test-only features.
+/// Using @unchecked because the class contains mutable state that is only accessed
+/// in a controlled manner through test-only methods.
     extension DefaultTyping: @unchecked Sendable {}
 #endif

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro